Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix path::deep_dependencies_trigger_rebuild often failing in CI #5952

Merged

Conversation

dwijnand
Copy link
Member

A shallow fix for path::deep_dependencies_trigger_rebuild in particular as it's failed my PRs often.

See #5935 (comment), and #5940 for the bigger picture.

@rust-highfive
Copy link

r? @matklad

(rust_highfive has picked a reviewer for you, use r? to override)

@ehuss
Copy link
Contributor

ehuss commented Aug 30, 2018

👍 This should fix this particular test.

@dwijnand
Copy link
Member Author

r=ehuss?

@alexcrichton
Copy link
Member

Hm is this the right fix? The test here is that if we change a file it'll cause a rebuild, but the sleep has to be before the file is edited to simulate time passing between the build and the edit?

@ehuss
Copy link
Contributor

ehuss commented Aug 30, 2018

I'm not sure I understand. Any change that happens between builds should trigger a rebuild, regardless of when it is made. The problem is in this test:

  1. Build foo/bar/baz
  2. Modify baz
  3. Build foo/bar/baz
  4. Modify bar
  5. Build foo/bar

If step 2 is done immediately before 3, the mtime of baz.rs is N and its output is also N. Then, in step 5, it checks baz and because N==N it recompiles baz when it shouldn't.

If you sleep after the files are modified, then the output mtime for baz will be N+1 in step 3, so it won't recompile baz in step 5 because N < N+1.

@alexcrichton
Copy link
Member

Ah ok I think I misunderstood! I wrote up some more thoughts in #5940 about a longer-term solution, but otherwise this looks good!

@bors: r+

@bors
Copy link
Collaborator

bors commented Aug 30, 2018

📌 Commit 274c162 has been approved by alexcrichton

@bors
Copy link
Collaborator

bors commented Aug 30, 2018

⌛ Testing commit 274c162 with merge e954520...

bors added a commit that referenced this pull request Aug 30, 2018
…uild, r=alexcrichton

Fix path::deep_dependencies_trigger_rebuild often failing in CI

A shallow fix for `path::deep_dependencies_trigger_rebuild` in particular as it's failed my PRs often.

See #5935 (comment), and #5940 for the bigger picture.
@bors
Copy link
Collaborator

bors commented Aug 30, 2018

☀️ Test successful - status-appveyor, status-travis
Approved by: alexcrichton
Pushing e954520 to master...

@bors bors merged commit 274c162 into rust-lang:master Aug 30, 2018
@dwijnand dwijnand deleted the fix-path-deep_dependencies_trigger_rebuild branch August 30, 2018 19:39
@ehuss ehuss added this to the 1.30.0 milestone Feb 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants